Skip to content

Fix several occurences of the improper_ctypes lint in tests where that was not the intent #111972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Fix several occurences of the improper_ctypes lint in tests where that was not the intent #111972

wants to merge 7 commits into from
+247 −251

Conversation

asquared31415
Copy link
Contributor

Addresses most of #53858

Summary of changes:

allow(improper_ctypes) removed (it was not doing anything anymore)

  • tests/ui/lint/lint-ctypes-fn.rs
  • tests/ui/abi/variadic-ffi.rs
  • tests/ui/abi/issues/issue-97463-broken-abi-leaked-uninit-data.rs
  • tests/ui/abi/foreign/foreign-fn-with-byval.rs
  • tests/debuginfo/type-names.rs
  • tests/codegen/riscv-abi/riscv64-lp64-lp64f-lp64d-abi.rs (note: it's suspicious that there's an empty struct here that works, why is that the case?)

Lint removed by addressing the root problem

  • tests/ui/issues/issue-3656.rs
  • tests/ui/issues/issue-51907.rs
  • tests/ui/repr/align-with-extern-c-fn.rs
  • tests/ui/lint/clashing-extern-fn.rs
  • tests/ui/lint/clashing-extern-fn-recursion.rs
  • tests/ui/abi/issue-28676.rs
  • tests/ui/abi/issues/issue-62350-sysv-neg-reg-counts.rs
  • tests/ui/abi/extern/extern-pass-TwoU8s.rs
  • tests/ui/abi/extern/extern-pass-TwoU16s.rs
  • tests/ui/abi/extern/extern-pass-TwoU32s.rs
  • tests/ui/abi/extern/extern-pass-TwoU64s.rs
  • tests/ui/abi/extern/extern-return-TwoU8s.rs
  • tests/ui/abi/extern/extern-return-TwoU16s.rs
  • tests/ui/abi/extern/extern-return-TwoU32s.rs
  • tests/ui/abi/extern/extern-return-TwoU64s.rs
  • tests/ui/abi/abi-sysv64-register-usage.rs
  • tests/ui/cfg/conditional-compile.rs
  • tests/ui/abi/abi-sysv64-arg-passing.rs (note: one instance here is intentional in the test, can this be fixed?)
  • tests/ui/lint/clashing-extern-fn.rs (note: two instances here are intentional)

Intentional or the lint firing/not firing is part of the test

  • tests/ui/lint/warn-ctypes-inhibit.rs
  • tests/ui/issues/issue-16441.rs
  • tests/ui/lint/lint-ctypes.rs
  • tests/ui/abi/extern/extern-pass-empty.rs (note: #![allow(improper_ctypes)] // FIXME: this test is inherently not FFI-safe.)

Unknown/Not addressed in this PR/Needs more investigation

  • tests/ui/issues/issue-28600.rs (possibly intentional for ICE)
  • tests/ui/issues/issue-26997.rs (possibly intentional for ICE)
  • tests/ui/issues/issue-38763.rs (possibly intentional for ICE)
  • tests/ui/issues/issue-5754.rs (possibly intentional for ICE)
  • tests/ui/foreign/nil-decl-in-foreign.rs (maybe an LLVM bug test? It was fixed with no links to how.)
  • tests/ui/mir/mir_cast_fn_ret.rs (I don't know enough about this test)
  • tests/ui/mir/mir_codegen_calls.rs (I don't know enough about this test)
  • tests/ui/abi/numbers-arithmetic/i128-ffi.rs (note: it's testing 128-bit ints in FFI, this is a known problem cc i128 / u128 are not compatible with C's definition. #54341 128-bit integers marked as having an unknown stable ABI #78473 )

@rustbot
Copy link
Collaborator

rustbot commented May 26, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 26, 2023
@asquared31415
Copy link
Contributor Author

@rustbot author whoops, meant to r? ghost in the first place, I want to do some more looking into some of the final list

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 26, 2023

Failed to set assignee to ghost: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@petrochenkov
Copy link
Contributor

petrochenkov commented May 26, 2023 •
edited
Loading

You can keep me as a reviewer for this PR.
I'd expect majority of allow attributes in tests/ui to be removable, not only improper_ctypes, and that would generally improve quality of the tests, but keeping this PR focused on improper_ctypes is better because it will already become pretty large with this lint alone.

@petrochenkov petrochenkov marked this pull request as ready for review May 26, 2023 00:57
@bors
Copy link
Collaborator

bors commented Jul 3, 2023

☔ The latest upstream changes (presumably #108611) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@asquared31415 any updates on this?

@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2023
@asquared31415 asquared31415 deleted the proper_ctypes branch December 3, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@asquared31415 @rustbot @petrochenkov @bors @Dylan-DPC